Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#270 - Add middleware to refresh access token #343

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Dejiah
Copy link

@Dejiah Dejiah commented Jul 2, 2024

Hi,

as mentioned in #270 , I think this would be a great addition to the library and the other PR ( #278 ) is stale, so I am trying to take this over the finish line.

I tried to address your comments as far as possible.

You mentioned in the review for the stale PR that you would not add the access token to the session "just for the sake" of it.
I kept this part of the code intentionally, because I would argue that having access to the access token within the request(-session) is an important feature.
I think the most important use case is to be able to use the access token for subsequent requests on behalf of the user to other services.

In our specific situation, we would like to use django-auth-adfs in a situation, where we have a Django Frontend that needs to perform the user sign-in (via Azure AD) and then request multiple downstream services on behalf of the user to acquire data to show. As all of these downstream services are in-turn OAuth2 protected, we need to send a valid access token with each request, which is only possible if we can access it outside of the AuthBackend e.g. via the session.

I also checked the solution proposed in #267 but I believe that extending the User model and thus saving the access token in the database leads to a security vulnerability as in this case anyone having access to the database could perform requests on behalf of every logged-in user.

Let's discuss and find a solution :-)

@tim-schilling
Copy link
Member

I kept this part of the code intentionally, because I would argue that having access to the access token within the request(-session) is an important feature. I think the most important use case is to be able to use the access token for subsequent requests on behalf of the user to other services.

If you didn't have the access token from here, how would you implement it otherwise in the application (not this library)? I'm not convinced that this package should handle anything other than authentication (not authorization or integration with Azure web services).

@Dejiah
Copy link
Author

Dejiah commented Jul 2, 2024

Thanks for you swift reply!

I kept this part of the code intentionally, because I would argue that having access to the access token within the request(-session) is an important feature. I think the most important use case is to be able to use the access token for subsequent requests on behalf of the user to other services.

If you didn't have the access token from here, how would you implement it otherwise in the application (not this library)? I'm not convinced that this package should handle anything other than authentication (not authorization or integration with Azure web services).

You would also have to carry out one of the OAuth2 authentication flows to retrieve an access token and thus authenticate the users, e.g. using the MSAL library (https://github.com/AzureAD/microsoft-authentication-library-for-python) in case of Azure AD. But this is exactly what you already do within the library and would mean duplicating the whole token exchange logic etc.

I think the point here is not yet about authorisation or integration yet as exposing the access token is simply the "source of truth" for authenticating the user. This is also what the library uses internally to derive the exposed user information.
However, in some cases (e.g. when one wants to forward the authentication to some other service) the exposed information, which you extract from the token, is not sufficient (e.g. because it is not signed and not verifiable by an external service).
Authorisation (i.e. if you are actually allowed to do something) and integrating with other services would still be out of scope for the library but would require to have this "pure" authentication available in some cases.

Merge in latest changes
@tim-schilling
Copy link
Member

The only reason that I can come up with to not store the token is to avoid allowing the library to do more with the API as a part of the library. But I don't think that's a good enough reason to make things more difficult for users. That said, I'm okay with storing it. cc @JonasKs

@tim-schilling
Copy link
Member

And now I'm having second thoughts around security concerns. If we start storing the access token in the session, we're making a choice for all users of the library. That shouldn't be something we decide for users unilaterally. Additionally, we can't use the session is the user is using cookie-based sessions.

The changes I'd like to see are:

  • Move as much of the new changes to the middleware and out of the backend
  • Only store the token information in the session if the refresh token middleware is being used (not sure the best way here)
  • Prevent the library from storing tokens in the session if cookie based sessions are being used
  • There needs to be some documentation

@Dejiah
Copy link
Author

Dejiah commented Jul 16, 2024

Thanks for the feedback!

I have some vacation coming up but will look into it again once I am back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants